-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid UriComponentsBuilder.fromUri #15853
base: main
Are you sure you want to change the base?
Avoid UriComponentsBuilder.fromUri #15853
Conversation
@bodograumann Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
@bodograumann Thank you for signing the Contributor License Agreement! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, @bodograumann! WIll you please also add a test for both servlet and reactive that show that this behavior is now repaired?
Thinking about this more, while it may still be valuable for this PR to have these tests, it seems to me that @rstoyanchev is open to updating |
True, fixing the underlying issue in UriComponentsBuilder would be preferrable, but I focused on what I could get done in limited time. |
The challenge with re-parsing the authority is having to extract the parsing logic out of the whole algorithm. Couldn't you re-parse the full URI string in fallback mode? Something like: URI uri = URI.create(issuer);
if (uri.getHost() == null && getAuthority() != null) {
uri = UriComponentsBuilder.fromUriString(issuer).build().toURI();
} This would impact only such URI's, and minimize any potential disruption for all others. |
That makes sense @rstoyanchev . |
Closes 15852
4416beb
to
ec73062
Compare
Given that since spring-projects/spring-framework@bbb53d0 As for testing: Could I make the |
The main difference is that @jzheaux note that this is in reference to a new URI parser that's used in If you decide to go with this change, why not pass |
Now I remember, that I tried doing something like that before, but |
Closes #15852
By using
UriComponentsBuilder.fromUriString
we stay in a single semantic context and do not loose information.